-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assert: check early in Eventually, EventuallyWithT, and Never #1427
base: master
Are you sure you want to change the base?
Conversation
This PR changes more than what it claims. Please keep it focused on #1424 and move the changes related to the tick separately. Because it seems that runs of the condition will not overlap anymore. |
d4ebae4
to
dc3af0f
Compare
I intend to merge #1395 first. Could you help with your own review now, and we will work on your own changes after the merge? |
assert/assertions_test.go
Outdated
condition := func(t *CollectT) { <-time.After(time.Millisecond) } | ||
|
||
done := make(chan struct{}) | ||
go func() { | ||
defer close(done) | ||
True(t, EventuallyWithT(mockT, condition, 1000*time.Millisecond, 100*time.Millisecond)) | ||
}() | ||
|
||
select { | ||
case <-done: | ||
case <-time.After(10 * time.Millisecond): | ||
Fail(t, `condition not satisfied quickly enough`) | ||
} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
assert/assertions_test.go
Outdated
condition := func() bool { <-time.After(time.Millisecond); return true } | ||
|
||
done := make(chan struct{}) | ||
go func() { | ||
defer close(done) | ||
True(t, Eventually(mockT, condition, 1000*time.Millisecond, 100*time.Millisecond)) | ||
}() | ||
|
||
select { | ||
case <-done: | ||
case <-time.After(10 * time.Millisecond): | ||
Fail(t, `condition not satisfied quickly enough`) | ||
} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dc3af0f
to
7ec44f8
Compare
// Check that a long running condition doesn't block Eventually. | ||
// See issue 805 (and its long tail of following issues) | ||
func TestEventuallyTimeout(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test isn't new; I just moved it so that all of the Eventually*
tests are grouped together rather than having the Never*
tests in between them
@cszczepaniak I just merged #1481 which uses |
@dolmen sounds good. I don't think that affects this PR, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for visibility
@cszczepaniak Please rebase as this requires to resolve merge conflicts. |
82097f1
to
bae586f
Compare
@dolmen Thanks, I've rebased now. |
Summary
Addresses #1424.
Eventually
andEventuallyWithT
current must always wait at least the polling duration before they can succeed. This PR starts checking the condition immediately. The assertion still fails if the initial check of the condition takes longer than the configured timeout.Changes
Motivation
This will provide a small optimization to callers, because tests can complete more quickly than they did before if conditions are met already.
Related issues